fix(local-network): cross-service SET, post-save IP redirect, pool prefix sync (#1039)#1117
Conversation
…efix sync (#1039) - Fix firmware error 7005 on save: LAN settings span multiple USP Services (Device.IP, Device.DHCPv4, Device.DeviceInfo), which the firmware cannot apply in one atomic SET. Pass allowPartial: true so each Service applies independently. - Add post-save redirect flow for IP changes: changing the router IP makes the old address unreachable and the SET response never returns, so treat an IP-changing SET as terminal (timeout / transport error = success; a real firmware fault still surfaces), redirect the browser to https://<hostName>.local, and intentionally drop SSE to suppress the recovery dialog. Triggered only on an IP address change — a mask-only or DHCP-only change is awaited normally with a success message. - Fix address pool prefix sync following IP or subnet mask changes: the pool prefix only re-synced when the IP changed, not the mask. Tightening the mask (e.g. /16 to /24) widened the locked prefix without updating the pool, leaving pool octets both out-of-subnet (validation error) and locked read-only — a dead end. Sync now runs on either change. - Add lanIpRedirect strings across all 26 locales. - Add tests: terminal-SET classification (timeout/transport = success, fault rethrows), redirect + SSE-disconnect branch, and pool prefix auto-sync. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
AustinChangLinksys
left a comment
There was a problem hiding this comment.
🤖 Automated Review — Round 1 · c4b6adb..746df56 (full)
Verdict: ✅ APPROVE — Logic is sound; timeout-as-success is correctly guarded for the IP-change case in the catch block, but the on TimeoutException clause itself lacks the guard. Three warnings worth addressing before ship.
| Conf. | Where | Issue (one-liner) | |
|---|---|---|---|
| 🟢 High | usp_local_network_service.dart:85 |
[both reviewers] on TimeoutException not guarded by ipAddressChanged — throttler timeout on non-IP-change saves silently treated as success |
|
| 🟡 Med | usp_local_network_view.dart:439 |
IP-change with empty hostName falls through to success snackbar, no redirect shown |
|
| 🟡 Med | lan_ip_redirect_dialog.dart:1-40 |
Near-verbatim duplicate of bridge_redirect_dialog.dart (DRY violation) |
|
| 🟡 Med | usp_local_network_service.dart:33 |
_ipChangeSetTimeout = Duration(seconds: 4) duplicated verbatim in usp_internet_settings_service.dart:43 |
|
| 🟡 Med | notifier_test.dart:307-370 |
Pool-prefix regression test instantiates real UspLocalNetworkService inside a Provider-layer test (crosses layer boundary) |
|
| 💡 | 🟢 High | usp_local_network_service.dart:101 |
Transport-error guard uses is-type enumeration; future ServiceError subtypes won't auto-include |
| 💡 | 🟢 High | assign_ip.dart:19 |
dart.library.html conditional export; package:web uses dart.library.js_interop — should migrate |
| 💡 | 🟢 High | lib/l10n/ |
✅ All 26 ARB locale files consistently include the 3 new keys |
| 💡 | 🟡 Med | usp_local_network_view.dart:406-449 |
_onSave redirect-vs-snackbar branch logic has no Widget test coverage |
| 💡 | ⚪ Low | usp_local_network_notifier.dart:107-138 |
save() override doesn't call super.save() — intentional but undocumented deviation from mixin template-method contract |
Confidence: 🟢High = code-verified · 🟡Med = located + reasoned, not fully confirmed · ⚪Low = speculative, please double-check.
Items marked [both reviewers] were independently flagged by two agents → higher confidence.
⚠️ Warning Details
1. 🟢 [both reviewers] on TimeoutException not guarded by ipAddressChanged — usp_local_network_service.dart:85
// usp_local_network_service.dart:80-91
final result = ipAddressChanged
? await updateFuture.timeout(_ipChangeSetTimeout)
: await updateFuture; // ← if throttler times out here, TimeoutException propagates
_handleSetResult(result);
} on TimeoutException {
// Comment says "only reachable on IP change" — incorrect
logger.i('[USP][Network][LAN]: IP-change SET timed out after '
'${_ipChangeSetTimeout.inSeconds}s — treating as success ...');LanNetworkInfo.update() calls _resolveInstance(client) which calls client.get() — and get() routes through BridgeRequestThrottler. If the throttler's 15-second timeout fires during _resolveInstance on a non-IP-change save (e.g. DHCP pool or hostname only), a TimeoutException propagates out of updateFuture and is silently swallowed as "IP-change success", while the settings were never applied. The UI shows success; the router ignores the change.
Trigger: Any non-IP-change save when the router is busy and the bridge throttler times out (15s).
Fix:
} on TimeoutException {
if (!ipAddressChanged) rethrow; // throttler timeout on non-IP-change = genuine failure
logger.i('[USP][Network][LAN]: IP-change SET timed out — treating as success');
}2. 🟡 IP-change + empty hostName falls to success snackbar — usp_local_network_view.dart:439
if (ipChanged && hostName.isNotEmpty) {
await showLanIpRedirectDialog(context, hostName: hostName);
} else {
showSuccessSnackBar(context, loc(context).localNetworkSettingsSaved);
// ↑ this branch fires when ipChanged==true && hostName==""
}If the user changes the IP while the hostName field is empty (corner-case: validation should reject, but save() doesn't enforce validation), the IP is applied, the old address becomes unreachable, but the user sees a success snackbar instead of a redirect dialog.
Fix: Show redirect dialog whenever ipChanged, falling back to new IP address string if hostName is empty; or assert-guard that save() cannot be called with validation errors.
3. 🟡 lan_ip_redirect_dialog.dart is near-verbatim copy of bridge_redirect_dialog.dart
Diff between the two files shows 19/40 lines differ, and all differences are identifier strings (function name + 4 l10n keys). The entire structure, dismissible: false, AppButton.primary, assignWebLocation injection, URL construction — is identical. Future changes (e.g. adding a countdown timer, tweaking dismissible logic) must be applied twice.
Fix: Extract a shared showPostSaveRedirectDialog(context, {title, message, buttonLabel, hostName, navigate}) helper in lib/components/shortcuts/dialogs.dart or a new shared helper file.
4. 🟡 _ipChangeSetTimeout magic constant duplicated — usp_local_network_service.dart:33
// usp_local_network_service.dart:33
static const _ipChangeSetTimeout = Duration(seconds: 4);
// usp_internet_settings_service.dart:43
static const _bridgeSetTimeout = Duration(seconds: 4);Both represent the same physical concept: "maximum wait before firmware drops the connection after applying a new IP". Two independent static const values — if firmware behavior changes, both must be updated.
Fix: Extract const kTerminalSetTimeout = Duration(seconds: 4) to lib/core/usp/usp_constants.dart and import in both services.
5. 🟡 Provider-layer test instantiates real UspLocalNetworkService — notifier_test.dart:307-370
uspLocalNetworkServiceProvider
.overrideWithValue(UspLocalNetworkService(MockUspClient())), // real Service in Provider testlockedOctetCount and syncPrefix are currently pure functions (don't call _usp), so MockUspClient() is never invoked. But placing a real Service in a Provider-layer test crosses the architectural boundary: if those methods ever start using _usp, this test begins making unintentional network calls against an unstubbed mock.
Fix: Move this regression test to usp_local_network_service_test.dart where it tests service logic directly; or add a comment marking it as a deliberate cross-layer integration test.
✅ What looks good
- Terminal-SET pattern is correct: The
catch (e)block properly guards transport errors withipAddressChanged && (mapped is NetworkError || mapped is ConnectivityError)— this correctly distinguishes real firmware faults (ServiceError → rethrow) from expected disconnect-on-IP-change (return). _handleSetResultcorrectly handlesUspPartialSuccess:UspPartialSuccessthrowsUspPartialFailureError(aServiceError) which is re-thrown by the catch block — no silent partial failures.allowPartial: trueis safe here: The_handleSetResultswitch throws onUspPartialSuccess, so partial failures surface as errors.- SSE disconnect uses
intentional: true:sseManagerProvider.disconnect(intentional: true)prevents auto-reconnect during IP-change redirect window. - Pool prefix sync triggers on both IP and mask change: The
ipChanged || maskChangedguard correctly captures the broader case (PR's stated intent). - Pre-save snapshot of
ipChanged/hostName/networkChanged: Captured beforesave()correctly, somarkAsSaved()collapsing the diff doesn't affect redirect logic. - L10n is complete: All 26 locale ARB files contain
lanIpRedirectTitle,lanIpRedirectMessage,lanIpRedirectButton; metadata only inapp_en.arb(correct per ARB spec). - Test coverage: 319 lines of new tests covering timeout-as-success, transport-error-as-success, pool-prefix sync regression, SSE disconnect, and dialog widget rendering.
- Architecture hard rules all pass: L1 not autoDispose ✅, L2 uses
ref.readnotref.watch✅, mutation wrapped inuspMutationLockProvider.withLock()✅, codegen only imported by Service layer ✅, View usesui_kit_librarycomponents ✅.
Cross-reviewed by two independent agents (security+correctness / architecture+maintainability). Automated — please sanity-check before merge.
What this branch does
Fixes local network settings save (issue #1039) across three problems found while investigating the firmware error
7005:1. Cross-service SET (firmware error 7005)
LAN settings span multiple USP Services —
Device.IP,Device.DHCPv4,Device.DeviceInfo— which the firmware cannot apply in one atomic SET (allow_partial=falseacross more than one Service → 7005). The save now passesallowPartial: trueso each Service applies independently.2. Post-save redirect on IP change
Changing the router IP makes the old address unreachable and the SET response never returns. The IP-changing SET is now treated as terminal: a timeout / transport error is treated as success, while a real firmware fault still surfaces. After a successful IP change the app redirects the browser to
https://<hostName>.localand intentionally drops SSE to suppress the recovery dialog. This is triggered only on an IP address change — a mask-only or DHCP-only change is awaited normally with a success message.3. Address pool prefix sync
The pool prefix only re-synced when the IP changed, not the subnet mask. Tightening the mask (e.g. /16 → /24) widened the locked prefix without updating the pool, leaving pool octets both out-of-subnet (validation error) and locked read-only — a dead end. Sync now runs on either change.
l10n
Adds
lanIpRedirect*strings across all 26 locales.Tests
.localURL, button navigates, no dismiss action.Known limitation (follow-up: #1116)
Changing the LAN IP via USP does not trigger client re-DHCP (no link down/up / port bounce) the way bridge mode does, so a client stays on the old subnet until its DHCP lease expires — the redirect only works once the client picks up a new lease. Tracked as a firmware-side fix in #1116.
🤖 Generated with Claude Code